-
Notifications
You must be signed in to change notification settings - Fork 351
Have profilers emit settings and metrics into event.json
#6597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6597 +/- ##
==========================================
- Coverage 83.54% 83.30% -0.24%
==========================================
Files 504 504
Lines 21191 21212 +21
==========================================
- Hits 17703 17670 -33
- Misses 3488 3542 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overall package sizeSelf size: 12.82 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.2.1 | 20.64 MB | 20.65 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.11.1 | 9.96 MB | 10.34 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.4 | 2.95 MB | 5.73 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @opentelemetry/resources | 1.9.1 | 306.54 kB | 1.74 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api-logs | 0.205.0 | 201.51 kB | 1.42 MB | | @opentelemetry/api | 1.9.0 | 1.22 MB | 1.22 MB | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.14.4 | 123.18 kB | 851.76 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | @datadog/openfeature-node-server | 0.1.0-preview.10 | 95.11 kB | 401.46 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2025-10-15 08:13:28 Comparing candidate commit 0f70dda in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1601 metrics, 69 unstable metrics. |
495c192 to
d318f4d
Compare
This comment has been minimized.
This comment has been minimized.
| // transpose e.g. info.wall.settings to info.settings.wall. That way | ||
| // we have functional groupings first, then profiler type. | ||
| infos.settings[type] = info.settings | ||
| delete info.settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems dangerous to modify the passed object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. It happens infrequently enough that it's not necessary to optimize for this.
| delete info.settings | ||
| // take over rest of info without transposition | ||
| if (Object.keys(info).length > 0) { | ||
| infos[type] = info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional grouping here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought not, the rest are typically metrics, so they'll end up as
wall.totalAsyncContextCount. Of course, we could embed this into another level, e.g. metrics so then it'd become wall.metrics.totalAsyncContextCount and we could then transpose that to metrics.wall.totalAsyncContextCount. FWIW, we don't even need to do the whole transposition, it just adds complexity; I originally wanted a top-level settings key because that's how e.g. Ruby does it, so I wanted to keep it a bit consistent with it:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree better be consistent with other langages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually now reworked it so that settings are constructed by config.js and embedded in system info by top-level profiler inprofiler.js and very little is left in individual profiler facets, just computed settings and metrics, and for those I don't bother with transposition anymore. I think it's much better this way, but I needed you asking to nudge me into rethinking it :-)
d318f4d to
590378f
Compare
590378f to
98b25d8
Compare
98b25d8 to
c09e149
Compare
What does this PR do?
Profiler uploads contain a file
event.jsonthat has a section for system info. The contents of system info are often language specific. So far we had the agent exporter set some system-specific values in it, but we want to generalize it a bit so that both the profiler config as well as individual profiler facets (wall, heap, events) can contribute values there as well. The config contributes its own settings, while the facets contribute either metrics or computed settings.In this iteration, the wall profiler contributes metrics for tracking the usage of async sample contexts when using
AsyncContextFrameon Node.js 24+.Additionally, the wall profiler now emits telemetry about the async sample contexts as well. This telemetry is defined in https://github.com/DataDog/dd-go/pull/190144
The PR contains commits that also make fields private in various touched classes. I also noticed while examining candidate for emitted settings that the stack depth in
space.jswill always be the hardcoded 64 as there's no external way to set it. Hardcoding it allows us to presume the value in the backend, which has some special handling for truncated frames.Motivation
Having more information in uploaded profiles about the system configuration + having insight into the use of async sample contexts.
Plugin Checklist
Additional Notes
Jira: PROF-12666